-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[LSP] Modify semanticTokens isFinalized
logic
#60484
[LSP] Modify semanticTokens isFinalized
logic
#60484
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will need to target 17.2
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs
Outdated
Show resolved
Hide resolved
This part is not true prior to #60485 |
// If the project's compilation isn't yet available, kick off a task in the background to | ||
// hopefully make it available faster since we'll need it later to compute tokens. | ||
if (!context.Document.Project.TryGetCompilation(out _)) | ||
{ | ||
_ = context.Document.Project.GetCompilationAsync(cancellationToken).ConfigureAwait(false); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is supposed to be handled by BackgroundCompiler
and CompilationAvailableTaggerEventSource
. Is there a reason we want it here separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an issue in Razor where document.Project.TryGetCompilation(out var _)
was returning false even after ~5 mins of waiting. We thought kicking off a separate task at the beginning might solve this problem and allow compilation to return sooner.
cc @CyrusNajmabadi is this the correct way to handle things?
var isFinalized = document.Project.TryGetCompilation(out var compilation) && compilation == semanticModel.Compilation; | ||
document = frozenDocument; | ||
document = document.WithFrozenPartialSemantics(cancellationToken); | ||
options = options with { ForceFrozenPartialSemanticsForCrossProcessOperations = true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ Do we want to use frozen partial semantics with isFinalized
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I wouldn't think so? I believe we want to call TryGetCompilation
on the complete document. The only other place the frozen document would be used with isFinalized
is when retrieving the WorkspaceStatusService, and unless I'm mistaken it doesn't seem like frozen partial semantics would be useful for that.
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this looks mostly good - just one potential race condition we should fix (should hopefully be straightforward with a lock)
// If the project's compilation isn't yet available, kick off a task in the background to | ||
// hopefully make it available faster since we'll need it later to compute tokens. | ||
var newCompilationTask = project.GetCompilationAsync(cancellationToken); | ||
_projectIdToCompilation.Add(project.Id, newCompilationTask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can technically race and lead to multiple calls to _projectIdToCompilation.Add
which would throw -
- 2 semantic tokens requests come in for the same project (could be diff docs).
- They run in parallel since semantic tokens isn't mutating
- Running in parallel, they both hit the if
!_projectIdToCompilation.ContainsKey(project.Id)
, and they both say it is not present in the dictionary (because neither has created the task yet). - Now both will try to add to the dictionary and potentially throw.
I think you'll want to guard access to the _projectIdToCompilation
with a lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Thanks
* Simplify * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Excluded Task.CompletionTask from completion list in async member declarations * Fix merge * Use existing assert * Use correct assert * Use isInferred: false when creating BoundArrayInitialization in CreateUTF8ByteRepresentation * Move few 'CodeFixProvider's to Analyzers layer * Fix * Fix * Remove ForegroundThreadAffinitizedObject usage * fix * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Revert bad changes * Fix errors and remove a hard CodeFixProvider back to Features * Update projitems * Move tests * Remove unused abstract property * Remove the single unused read of CodeFixCategory * Remove overrides in Analyzers * Remove overrides in Features * Fixed char recomendations test * Got rid of AsyncModifierName constants * Remove unnecessary `<Compile Remove` * Fixed tests for all other keywords * Cleaned up keyword tests * Move formatting analyzer to shared Analyzers layer * Move tests to shared Analyzers layer and more other fixes * Few more fixes * Fix bad changes * Fix bad changes * Fix bad changes * Remove IVTs of deleted test projects * Address review comment * Fix failing test * Cleanup unused resources in Features layer * Update Roslyn.Diagnostics.Analyzers and remove RS0005 suppressions * Move to SyntaxEditorBasedCodeFixProvider * Remove some 'MyCodeAction's * Remove more 'MyCodeAction's * One more that's not named MyCodeAction * Fix up changes between main and main-vs-deps * Colorize async as keyword in some cases * Few fixes * Added a bunch of tests, where async can never be a keyword, moved logic one level up * Fix typo * Note auto-default merged in feature status doc (#60564) * Update PublishData.json for 17.3 P1 (#60559) * Fix formatting * Added case with equals syntax, moved logic to a separate method * Add Rebuild badge to README (#60298) * Avoid using --blame-crash with CollectDumps This flag causes the test harness to attach ProcDump, which removes our ability to collect heap dumps on crash using Windows' built-in functionality. * Ensure current OOP calls for the same solution-checksum can share the same OOP solution computation * Update src/Workspaces/Remote/ServiceHub/Services/DocumentHighlights/RemoteDocumentHighlightsService.cs * Update src/Workspaces/Remote/ServiceHub/Services/SemanticClassification/RemoteSemanticClassificationService.cs * Revert name * Update tests * Fix * Add comment * Add comment * Update src/Workspaces/Remote/ServiceHub/Services/CodeLensReferences/RemoteCodeLensReferencesService.cs * fix grammar * Formatting * Revert * revert * Revert * Fix the Compiler VSIX extension so it works with CPS projects again This has been broken for quite some time. We were MEF importing a type that was no longer a MEF part, and referencing very old reference assemblies that needed to be updated as a part of responding to that original breaking change. * Make NotificationOption2 a serializable struct (#60573) * Update status for UTF8 literals and checked operators (#60587) * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800 * Simplify * Simplify * Simplify * Simplify * [LSP] Modify semanticTokens `isFinalized` logic (#60484) * try/finally * Simplify * Move comment * Simplify * Doc * Doc * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033 * Workspace config options (#59790) * REnames * Expand tuple * Rename parameter * Use deconstruction * Fix merge from main * Do not assert we are on the main thread in the constructor * Add support for unsafe local functions in IDE0062 code fix (#59297) * Track "numeric IntPtr" feature (#60579) * Address review comment * Add helpers * Remove unneeded IVTs * Simplify * Simplify * Pause computation of navbar items when a buffer is not visible * Rename * PR feedback part 1 * Change insertion branch * Update PublishData.json * PR feedback part 2 * lint * Implement handling of rename intent (#59410) * Implement handling of rename intent * Use documentId instead of URI * Move record definition up top * feedback * Address feedback * Remove unused files in CodeStyle layer * Remove unused resources in CodeStyle layer * Changed algorithm of detecting IAsyncEnumerable and IAsyncEnumerator, reverted some refactoring * Got rid of IsSystemRuntimeOrMscorlibAssembly method * Fix crash when combining reordered arguments with params array (#60622) * Fix string * Always use text edits in completion for correctness (#60466) * Filter down to task-like types when in an async context * Update providers * Cleanup * Revert * Revert * Revert * Simplify * Restore logic * Update test * Pull feature-specific APIs out of ISyntaxFacts * Extract out IBlockFacts out of ISyntaxFacts * Simplify * Instrument ReferenceCachingCS for flakiness (#60585) * Fix our INavigableSymbol to not root an entire solution-snapshot * Remove the TEmporaryWorkspace entirely from the RemoteWorkspace code. * Lint * Fixed adding a redundant blank line when suppressing with local attribute * [main] Update dependencies from dotnet/source-build-externals (#60359) [main] Update dependencies from dotnet/source-build-externals * Fix * Added more tests & refactoring * Clean up syntax context * Clean up syntax context * Clean up syntax context * Clean up syntax context * Simplify * Delay starting the work to scan for todo-items * move properties * Cleanup * Cleanup * Update tests * Fixup * Run continuation to dispose of cancellation token source (#60653) * Run continuation to dispose of cancellation token source * Remove cancellation token and execute synchronously * Update src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.cs Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com> Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com> * Global indentation options - take 2 (#60565) * Revert "Revert "Global indentation options (#59679)" (#60199)" This reverts commit ab57ce8. * Move IFormattingInteractionService to Editor Features and add inferred indentation detection to its implementation. Move GetFormattingChangesOnTypedCharacterAsync, GetFormattingChangesOnPasteAsync to ISyntaxFormattingService - these do not depend on the editor. * Test fix * Update struct field definite assignment tests * Restore CodeStyle test projects * Lint * Fallout * Pass options to FixAllAsync, simplify CodeAction registration (#60665) * Trim unnessasary leading lines when removing usings (#60672) * Formatting and code generation options (#60127) * Remove duplicate package references (#60658) * Remove duplicate package references * Delete additional Tools.props * Update for definite assignment changes Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com> Co-authored-by: Joey Robichaud <jorobich@microsoft.com> Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com> Co-authored-by: DoctorKrolic <mapmyp03@gmail.com> Co-authored-by: David Barbet <dabarbet@microsoft.com> Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com> Co-authored-by: Youssef1313 <youssefvictor00@gmail.com> Co-authored-by: DoctorKrolic <70431552+DoctorKrolic@users.noreply.github.com> Co-authored-by: Manish Vasani <mavasani@microsoft.com> Co-authored-by: Jason Malinowski <jason.malinowski@microsoft.com> Co-authored-by: Rikki Gibson <rigibson@microsoft.com> Co-authored-by: Allison Chou <allichou@microsoft.com> Co-authored-by: Sam Harwell <Sam.Harwell@microsoft.com> Co-authored-by: Tomáš Matoušek <tmat@users.noreply.github.com> Co-authored-by: msftbot[bot] <48340428+msftbot[bot]@users.noreply.github.com> Co-authored-by: Julien Couvreur <jcouv@users.noreply.github.com> Co-authored-by: Gérald Barré <meziantou@users.noreply.github.com> Co-authored-by: Jonathon Marolf <jmarolf@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com> Co-authored-by: Fredric Silberberg <frsilb@microsoft.com>
isFinalized
variable is used by Razor to determine whether to cache C# tokens on their end. C#'s tokens may not always be finalized since we utilize frozen partial semantics.isFinalized
's value involves callingTryGetCompilation
. However, we've run into problems sinceTryGetCompilation
is never guaranteed to actually return a compilation.TryGetCompilation
we now callIsFullyLoaded
and check to see if we have a full compilation. This isn't guaranteed to return finalized tokens but has been determined to be an appropriate workaround until workspace/semanticTokens/refresh messages are implemented in Roslyn and Razor.